-
-
Notifications
You must be signed in to change notification settings - Fork 8.8k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Actually adjust copy button logic to new API #8554
Conversation
Yay, your first pull request towards Jenkins core was created successfully! Thank you so much! |
Remove the obsolete textarea creation and correctly handle other permission errors
HTMLUnit now supports `isSecureContext`
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks OK from security POV.
HTMLUnit 3 should already be implemented in all the plugins that are included in the bill of materials, but it is better to verify in the plugin bill of materials now than to discover it with a broken build of the plugin BOM at release time. Checking * jenkinsci/jenkins#8554 Thanks to @D3SOX for the pull request proposing the improvement.
Thanks very much @D3SOX . I've submitted a verification test with the plugin bill of materials to confirm that this improvement does not surprise us in the bill of materials release. Pull request is: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I tested the copy button on an SSL enabled Jenkins behind an NGINX reverse proxy and it worked great. Approved. Thanks very much.
Tests also ran as expected on the Jenkins plugin bill of materials. No test failures reported.
/label ready-for-merge This PR is now ready for merge, after ~24 hours, we will merge it if there's no negative feedback. Thanks! |
Congratulations on getting your very first Jenkins core pull request merged 🎉🥳 |
@@ -4,31 +4,20 @@ Behaviour.specify( | |||
0, | |||
function (copyButton) { | |||
copyButton.addEventListener("click", () => { | |||
// HTMLUnit 2.70.0 does not recognize isSecureContext |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
introduced in #7784 FTR; see HtmlUnit/htmlunit@54d9d30
hoverNotification(copyButton.getAttribute("message"), copyButton); | ||
if (isSecureContext) { | ||
// Copy the text to the clipboard | ||
navigator.clipboard |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
whilst mainstream browsers support this (according to canIUse) I believe unless you set a handler in HtmlUnit
that navigator.clipboard
will be undefined when running tests with webclient.
the webclient created in j-t-h is not configured with a handler.
The previous code (introduced in #6698) contained obsolete logic from the deprecated clipboard API, which was removed here.
The Promise returned by
navigator.clipboard.writeText
is now correctly handled as described per https://developer.mozilla.org/en-US/docs/Web/API/Clipboard/writeText#return_valueI've also removed the check if it's running as test as HTMLUnit 3.1.0 added support for
isSecureContext
Testing done
I have overwritten the function with the console to always return a rejecting Promise. Therefore, the new logic is tested
Proposed changelog entries
I think this does not need to be mentioned in the changelog
Alternatives I have considered
We could implement the deprecated clipboard API again when we are not in a secure context
Additional testing done